-
Notifications
You must be signed in to change notification settings - Fork 296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Unreal API for terrain height querying #1421
Conversation
This is ready to review! |
It's taking ~30 seconds to generate distance fields for built-in meshes (not our tiles) during automated test runs on CI, and if this happens at the wrong time it can cause async tests to time out and fail. E.g.: ``` [2024.09.19-13.55.42:741][157]LogMeshUtilities: Finished distance field build in 14.6s - 126x126x126 sparse distance field, 1.9Mb total, 0.1Mb always loaded, 52% occupied, 1322 triangles, EditorSkySphere [2024.09.19-13.55.57:236][896]LogMeshUtilities: Finished distance field build in 14.3s - 126x126x126 sparse distance field, 1.9Mb total, 0.1Mb always loaded, 52% occupied, 1322 triangles, SM_SkySphere [2024.09.19-13.55.58:335][ 28]LogMeshUtilities: Finished distance field build in 1.0s - 42x42x42 sparse distance field, 0.1Mb total, 0.0Mb always loaded, 96% occupied, 16 triangles, Cube [2024.09.19-13.55.59:518][170]LogMeshUtilities: Finished distance field build in 1.2s - 42x42x42 sparse distance field, 0.1Mb total, 0.0Mb always loaded, 96% occupied, 320 triangles, Sphere [2024.09.19-13.56.00:710][313]LogMeshUtilities: Finished distance field build in 1.0s - 42x42x42 sparse distance field, 0.1Mb total, 0.0Mb always loaded, 96% occupied, 170 triangles, Cylinder [2024.09.19-13.56.05:147][845]LogAutomationController: Error: Test Completed. Result={Fail} Name={works with a single position} Path={Cesium.Unit.SampleHeightMostDetailed.Cesium World Terrain.works with a single position} ```
Or... not quite ready for review. The new tests are taking an extraordinarily long time on CI - over 30 seconds when they just take a few milliseconds locally! My best guess is that the Cesium3DTileset is either not ticking, or it's ticking much more slowly, and so the height queries are taking ages. |
The test failures in UE 5.3 and 5.4 are caused by this previously-discovered problem: |
The problem mentioned above has been fixed, so this is ready to review. |
This seems to work well! I'm still adding some comments but the results look great so far. In our Also tested what happens if you give the function a position that isn't on the tileset, and it correct returns false for the sample's success. Here's the Blueprint I used if anyone wanted it for reference: I also noticed that the performance tests take between 6-10 seconds, is that expected? I thought it might have to do with the cache, but even with a warm cache it took around 5-6 seconds on my machine. Maybe I had too many browser tabs open 😛 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! I didn't pore over the changes to the test code, but I verified that the tests pass on my machine and that the behavior works.
My comments are mainly doc / naming tweaks. To speed things up for the release, I'm going to implement most of them myself, but I will leave the open-ended ones up in case you have opinions @kring .
Source/CesiumRuntime/Public/CesiumSampleHeightMostDetailedAsyncAction.h
Outdated
Show resolved
Hide resolved
Source/CesiumRuntime/Public/CesiumSampleHeightMostDetailedAsyncAction.h
Outdated
Show resolved
Hide resolved
Source/CesiumRuntime/Public/CesiumSampleHeightMostDetailedAsyncAction.h
Outdated
Show resolved
Hide resolved
That doesn't sound too far out of line. The tests are doing 400 height queries, spread out over a large enough area that it requires loading quite a few tiles, especially in the GP3DT case. |
I think I've addressed everthing, this is ready for another look. |
@kring the changes look good, but I'm still seeing the same behavior with the tileset not being valid. :( I set the ion Asset ID of a tileset to be 0, which is not a valid tileset. I stepped through the code and found that
|
Other than that, this can be merged! If it can't be fixed before release, no worries, we can note it as a TODO issue in cesium-native. |
@j9liu I believe asset ID 0, and also a blank URL, are a special case because we never even try to load a tileset in those states. Any other tileset failure should work as expected, I think. I can't quite decide if the behavior in the special case is a bug or a feature... Essentially it'll wait until those properties are set and then do the height query, rather than failing immediately. |
Ah okay @kring . I was also trying other bogus asset IDs aside from 0 (like 2, which is a raster overlay), but I can see them falling into the same category of "we never load this tileset". These were just the easiest "failures" I could simulate, that's why I seem fixated on them 😛 If there's any other failures I should / could test, definitely let me know |
Asset ID 2 should fail. If it's not, something is wrong. I'll check it out! |
Builds on cesium-native PR 783, so merge that first.
Introduces new test to verify results and measure performance of new cesium-native terrain height query functionality.
(using the existing performance test framework)
Still a bit of work to do here...